SOLR-18212: Migrate PackageAPI endpoints from @EndPoint/@Command to JAX-RS#4178
SOLR-18212: Migrate PackageAPI endpoints from @EndPoint/@Command to JAX-RS#4178epugh wants to merge 22 commits intoapache:mainfrom
Conversation
- Replace PackagePayload.AddVersion with AddPackageVersionRequestBody
- Move package name from body to URL: POST /cluster/package/{name}/versions
- Replace delete command pattern with DELETE /cluster/package/{name}/versions/{version}
- Replace refresh command with POST /cluster/package/{name}/refresh
- Update errPath in testAPI from /details[0]/errorMessages[0] to /msg
- Remove all add.pkg assignments (pkg now lives in URL path)
- Replace add.pkg references in verifyComponent() with literal package name strings
- Update import: remove PackagePayload, add AddPackageVersionRequestBody (sorted)
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: epugh <22395+epugh@users.noreply.github.com>
…sponse.error field Co-authored-by: epugh <22395+epugh@users.noreply.github.com>
Co-authored-by: epugh <22395+epugh@users.noreply.github.com>
Co-authored-by: epugh <22395+epugh@users.noreply.github.com>
|
Is there a JIRA ticket associated with this change @epugh? |
No, because I viewed it as "just a refactoring and no one will notice....", and that I was hoping there were no debates that would require a JIRA to hash out! |
| @@ -0,0 +1,7 @@ | |||
| title: Migrate PackageAPI to JAX-RS. PackageAPI now has OpenAPI and SolrJ support. | |||
There was a problem hiding this comment.
[-0] A user likely doesn't care what framework they APIs are written in. But they probably do care that some of the actual API paths have changed, there's new SolrJ classes for these APIs, etc.
| authors: | ||
| - name: Eric Pugh | ||
| links: | ||
| - name: PR#4178 |
There was a problem hiding this comment.
[0] Probably worth having a JIRA reference for this guy, even if it's just a tie-back to one of the v2 umbrella tickets? It's changing APIs and stuff, so worth tracking in more than just the PR description I'd imagine.
| summary = "List all packages registered in this Solr cluster.", | ||
| tags = {"package"}) | ||
| PackagesResponse listPackages( | ||
| @Parameter(description = "If provided, the named package is refreshed on this node.") |
There was a problem hiding this comment.
[Q] The "named package"? what named package? This API doesn't specify a package name anywhere afaict?
Did you mean to put this param on the 'getPackage' method below, which does have a packageName?
There was a problem hiding this comment.
so, no, this is correct. It's used internally to signal between nodes that they need refreshing. I am digging more in and I think the description is bad.
| @Operation( | ||
| summary = "Get information about a specific package in this Solr cluster.", | ||
| tags = {"package"}) | ||
| PackagesResponse getPackage( |
There was a problem hiding this comment.
[Q] Should 'expectedVersion' be supported here, in addition to the "list" method above?
In the current code on main it looks like the query param is supported for both the "list-all" and "get-single-package" usecases.
There was a problem hiding this comment.
Yep! It's kind of internal tooling, but yeah.
| try { | ||
| V2Response resp = req.process(solrClient); | ||
| printGreen("Response: " + resp.jsonStr()); | ||
| new PackageApi.DeletePackageVersion(packageName, version).process(solrClient); |
There was a problem hiding this comment.
[+1] Love the V2Request removal!
|
|
||
| if (refreshPackage != null) { | ||
| packageStore.packageLoader.notifyListeners(refreshPackage); | ||
| return instantiateJerseyResponse(PackagesResponse.class); |
There was a problem hiding this comment.
[0] instantiateJerseyResponse is primarily useful for cases when:
- a response is filled out bit by bit
- it's possible for an exception to interrupt that process mid-flow, and
- when the response is sent back to the user, we want it to contain both the partial-success information as well as the error information.
A good example is CreateCollection. Create-collection fires off a bunch of core-creations and puts information for each (success or failure) into the user-facing response. Historically, if an error crops up in the middle of those core creations we want the response to return information both on the ones that succeeded and the ones that failed. That's what instantiateJerseyResponse is good for - it registers the response object in such a way that our exception handling can find the partial response and knit it together with information pulled out of the exception.
None of that seems relevant here (and elsewhere) - you should be able to replace this line with return new PackagesResponse();
There was a problem hiding this comment.
Fixing now! Thank you... Fixed a number of places with this pattern.
| public int hashCode() { | ||
| return Objects.hash(version); | ||
| } | ||
| final var request = |
There was a problem hiding this comment.
[Q] Is there a reason you're not using the auto-generated SolrJ type here? We should have one now that the API is JAX-RS!
If this was an intentional decision made to prevent scope-creep, I'm fine with that. Just making sure it's not oversight.
There was a problem hiding this comment.
so many places to update... I suspect in the future we need to do a "Check each GSR for fitness" check ;-)
| * <p>Note: SolrJettyTestRule cannot be used here because the Package API requires ZooKeeper for its | ||
| * cluster-level operations. A one-node SolrCloud cluster is used instead. | ||
| */ | ||
| public class PackageAPITest extends SolrCloudTestCase { |
There was a problem hiding this comment.
[Q] What's the difference between this class, and the existing TestPackages? They seem to overlap a good bit. Should they be combined perhaps?
Or put differently: if I'm adding a new test case a month from now how would I know whether it belongs in PackageAPITest or in TestPackages?
There was a problem hiding this comment.
You know, I was kind of wondering that myself. I suspect at the end of all of this, we'll want to come back and have a single strong pattern of what tests go where for each v2 api... they have grown organically.
There was a problem hiding this comment.
Okay, embarking on a reorg of the tests...
| List.of("/my-plugin/plugin-" + version + ".jar")))) | ||
| .build(); | ||
| processRequest(client, packageRequest); | ||
| var addRequest = new PackageApi.AddPackageVersion("mypkg"); |
| add.files = Arrays.asList(new String[] {FILE1}); | ||
| V2Request req = | ||
| new V2Request.Builder("/cluster/package") | ||
| new V2Request.Builder("/cluster/package/mypkg/versions") |
There was a problem hiding this comment.
Ditto, re: choosing to retain V2Request usage vs. replacing with the auto-generated type.
We don't need to make that switchover in this PR, just pointing it out in case it's unintentional.
There was a problem hiding this comment.
Originally I was trying to change as little as possible, but... It's so much fun to erase V2Request, and I suspoose it also gvies us testing over the generated code...
…kageapi-to-jax-rs
Rename PackageAPI to ClusterPackage to follow naming pattern of other V2 apis.
Add test, revamp description.
|
@gerlowskija I think I am ready for another review. I renamed |
Migrates
org.apache.solr.pkg.PackageAPIfrom Solr's homegrown@EndPoint/@Commanddispatch to standard JAX-RS annotations, consistent with other V2 APIs (ClusterFileStore,ZookeeperRead,ClusterProperty).This has a pretty hefty update to the patterns, but I thought it was better to do it now while v2 api's are "experimental".
New REST endpoints
The command-based POST pattern is replaced with RESTful routes:
POST /cluster/package {"add": {"package":"p","version":"v","files":[...]}}POST /cluster/package/{name}/versionsPOST /cluster/package {"delete": {"pkg":"p","version":"v"}}DELETE /cluster/package/{name}/versions/{version}POST /cluster/package {"refresh": "name"}POST /cluster/package/{name}/refreshGET /cluster/packageGET /cluster/package/{name}Key structural changes
PackageAPI— new JAX-RS implementation (extends JerseyResource implements PackageApis); replaces the oldEdit/Readinner classes (~220 lines removed)PackageStore— renamed from the oldPackageAPI; now clearly a ZK data/state holder (Packages,PkgVersion, ZK watcher), not an endpoint class. THIS WAS A BIG REFACTORINGsolr/apimodule — newPackageApis.java(JAX-RS interface),PackagesResponse.java,AddPackageVersionRequestBody.java; OAS generation producesPackageApiSolrJ request classes automaticallyPackageAPITest— integration test usingSolrCloudTestCase(ZK required) with generatedPackageApiSolrJ request classesTestPackages— updated to use new endpoints; error path changed from/details[0]/errorMessages[0]to/msgpackage-manager-internals.adoc— updated endpoint reference and curl examples